Replace ComponentSignals with IOPorts implementation#481
Conversation
PhilipFackler
requested review from
lukelowry,
nkoukpaizan and
pelesh
and removed request for
lukelowry
|
This looks awesome!! |
There was a problem hiding this comment.
So so helpful for my development. This is very intuitive and helps understand and debug intermodel behavior. I would like this to be merged before the new Phasor Dynamics models so I can use this syntax!
Masterful this helps so much with modeling
|
As a related question, for future development in EMT: how difficult would this be to implement for vector-typed signals instead of scalar-typed? In an ideal world, the best possible design for EMT is a N width signal (decided at runtime), which I have no clue how to implement. |
Handling the variable should be pretty simple. Especially if it can be wrapped in a non-owning vector. That's what The more challenging part would be the use of |
pelesh
left a comment
pelesh
left a comment
There was a problem hiding this comment.
This is certainly step in the right direction and makes component model signal connections more streamlined.
A few things that need to be addressed before this is ready for review:
- Component model constructors should take only reference to the component model data as input parameters. All signal connectivity information is already in there.
- Treat signal input and outputs separately as these are different types of ports (nodes).
- Implement different classes/structs in separate source files.
- Align naming of the object with what we have agreed earlier.
| template <typename scalar_type, typename index_type> | ||
| class Port | ||
| { | ||
| public: | ||
| /// Scalar type used for signals |
There was a problem hiding this comment.
Class Port must be in separate source file from IOPorts.
| void connect(SignalNodeT* node) noexcept | ||
| { |
There was a problem hiding this comment.
Node and port are often used interchangeably in nodal analysis. What is node in this case?
| template <typename scalar_type, typename index_type> | ||
| class OutputPort : public Port<scalar_type, index_type> | ||
| { |
There was a problem hiding this comment.
I believe we decided to name this SignalOut. This should also be in a separate file.
| /** | ||
| * @brief Port for receiving a signal from a SignalNode | ||
| */ | ||
| template <typename scalar_type, typename index_type> | ||
| class InputPort : public Port<scalar_type, index_type> | ||
| { | ||
| public: |
There was a problem hiding this comment.
Rename to SignalIn or SignalInput and implement in a separate file.
| template <typename scalar_type, typename model_data_type> | ||
| struct IOPorts | ||
| { | ||
| /// Scalar type used for signals | ||
| using ScalarT = scalar_type; |
There was a problem hiding this comment.
More intuitive name would be SignalPorts. Needs to be implemented in a separate file.
| exciter.getPorts().out[SexsPtiSignalOutputs::efd].connect(&efd_node); | ||
| exciter.getPorts().in[SexsPtiSignalInputs::vs].connect(&vs_node); |
There was a problem hiding this comment.
A better approach would be to have methods getSignalInput() and getSignalOutput()`.
| exciter.getPorts().out[SexsPtiSignalOutputs::efd].connect(&efd_node); | |
| exciter.getPorts().in[SexsPtiSignalInputs::vs].connect(&vs_node); | |
| exciter.getSignalOutput[SexsPtiSignalOutputs::efd].connect(&efd_node); | |
| exciter.getSignalInput[SexsPtiSignalInputs::vs].connect(&vs_node); |
Similar for other components.
| * A signal node conceptually carries a signal (scalar value) from an | ||
| * output port of one component to an input port of another component (or | ||
| * multiple components). | ||
| * | ||
| * (Component):[OutputPort] -> {SignalNode} -> [InputPort]:(Component) | ||
| * | ||
| * A SignalNode can be "connected" to a Port. When that port is an | ||
| * OutputPort, the SignalNode is considered `assigned()` since it can be | ||
| * connected to only one OutputPort. The SignalNode is considered `linked()` | ||
| * when the actual signal (scalar variable) has been made available. |
There was a problem hiding this comment.
There seems to be some confusion with naming scheme here: The output port is a signal node. What is called a SignalNode here is a signal link.
| IOPortsT& getPorts() | ||
| { | ||
| return signals_; | ||
| return ports_; | ||
| } |
There was a problem hiding this comment.
I would create separate methods for input and output signal ports. These are two different types of objects with different connection properties.
| Ieeest(const ModelDataT& data, SignalNodeSetT& signal_nodes); | ||
| ~Ieeest(); |
There was a problem hiding this comment.
The constructor should not take reference to SignalNodeSetT as the input parameter. All signal connectivity information is already in ModelDataT. Similar for other component models.
| Ieeest(const ModelDataT& data, SignalNodeSetT& signal_nodes); | |
| ~Ieeest(); | |
| ~Ieeest(); |
4 participants
Description
This is a refactor of
ComponentSignalsto be (hopefully) more intuitive and also make model development a bit easier.Proposed changes
Manage signal node connections via input and output ports
SignalNodeSetclass for managing the signal nodes for the system. This can then be passed to components since it doesn't require access to theSystemModelimplementation. This maps either signal_id or name to signal nodes.IOPortsautomatically connect signal nodes to ports at construction (rather than doing it manually inSystemModelComponentSignalsChecklist
-Wall -Wpedantic -Wconversion -Wextra.